-
Notifications
You must be signed in to change notification settings - Fork 100
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix duplicate navigation for lesson nav #2806
Conversation
8925326
to
4d465ec
Compare
Thanks @jasmussen. Does this match your expectations of the result? lesson.complete.mp4 |
Yes it does. There may still be optimizations to do with regards to the next/previous lesson buttons and where they sit, perhaps they are reduced to just "Next" and "Previous", and are arrows instead of the bigger buttons. But we're getting into a detail territory that isn't necessarily a blocker for this particular PR. |
@@ -0,0 +1,14 @@ | |||
function init() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This could be done with CSS — display:none
hides content from everyone, including screen readers.
.sensei-lesson-footer .screen-reader-text {
display: none;
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, yeah, I was initially thinking about completely removing it, so I went ahead and did it that way. Thanks!
2899198
Yeah, actually this is what I had in mind originally, but I also feel that using a disabled button could be acceptable, maybe just treating it as a button-like div, as with that commit, the screen reader wouldn't read it as a button, it would just read the text "lesson completed." But it might be a bit confusing to sighted users and adding a label is definitely better. |
I'll try this first. However, I was originally thinking about adding a green label somewhere to make it more obvious, for example like adding an icon like you did and making the entire title green. Also, the "completed" status icon in the sidebar should probably be green as well, to make it more consistent with other places. This can be done post-launch, though. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! I tested with VoiceOver and no extra buttons/links exist. I think the title update can be handled in a future PR. Good to merge pending the unused CSS removal.
|
||
.disabled { | ||
color: var(--wp--preset--color--charcoal-5) !important; | ||
border-color: var(--wp--preset--color--charcoal-5) !important; | ||
pointer-events: none; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you can remove this style, since the disabled button has been removed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was definitely thrown together in dev tools for a suggestion. I think this can wait until post-launch so we can get some proper design input for it. |
Opened an issue for this. |
Fixes #2782
This PR removes the entire screen-reader-text div - ie. the nav & the first "completed" button are removed.
Also added a visible "Previous Lesson" button to sensei lesson actions.
And since the style of the Sensei next lesson button cannot align well with the previous lesson button on smaller screens (either it doesn't automatically become two rows or it becomes unclickable), a new "Next Lesson" button is also added.
I haven't figured out how to indicate that the current lesson is already completed. Initially it was a button-like div, but it caused confusion, so I removed it. Any thoughts on this? @WordPress/meta-design
Screencasts
Before
Before.mov
After
After.mp4
Smaller screen